improve performance of glTFLoader._applyExtensions #15913
Conversation
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/15913/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/15913/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/15913/merge#BCU1XR#0 |
|
Visualization tests for WebGPU (Experimental) |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGL 1 have failed. If some tests failed because the snapshots do not match, the report can be found at If tests were successful afterwards, this report might not be available anymore. |
|
Visualization tests for WebGL 1 have failed. If some tests failed because the snapshots do not match, the report can be found at If tests were successful afterwards, this report might not be available anymore. |
|
Thanks for the PR. Just curious, have you seen measurable performance issues with this code? @alexchuber also recently identified an issue with the loader extension ordering that needs to be addressed. It might be good to think about both of these issues together before making a complete fix for both as they will likely touch the same code. |
| loaderProperty._activeLoaderExtensionFunctions = loaderProperty._activeLoaderExtensionFunctions || new Map(); | ||
| const activeLoaderExtensionFunctions = loaderProperty._activeLoaderExtensionFunctions; | ||
|
|
||
| if (!activeLoaderExtensionFunctions.has(functionName)) { |
There was a problem hiding this comment.
Unfortunately, this won't work. Indexing by only functionName and not ${extension.name}.${functionName} means that, if an object needs the same function applied to it from several extensions, only one will be applied.
E.g., a material using KHR_materials_volume will always also have either KHR_materials_transmission or KHR_materials_diffuse_transmission, both of which use the function _loadMaterialPropertiesAsync.
When fixing this issue, try testing with this asset. If it looks correct when loaded in the Sandbox, you've probably solved the issue.
I do agree that we should find a way to avoid recursively applying extensions, since it results in unexpected behavior with the order in which extensions are applied. E.g., we expect that KHR_materials_volume is applied after the two transmission extensions mentioned above. However, if you look at the order property of these 3 extensions, you'll see that it's wrong (it's reversed) and shouldn't possibly work.
But the recursion just conveniently masks this problem by un-reversing the order, thus making it correct. AKA it's two bugs cancelling each other out :)
There was a problem hiding this comment.
Oh, forgot to clarify:
You will need to update the order of extensions once the recursion has been removed.
At minimum, we know that volume will need to have an order higher than both transmission and diffuse transmission.
Cache extensions by
_getActiveExtensionsHasOwnFunctionfor a specific function name, cache extensions which are enabled and have the specific function， avoid repeated traversals
ILoaderProperty._activeLoaderExtensionFunctions
use
Mapto replace{}for ILoaderProperty._activeLoaderExtensionFunctions, to imporve has/set/delete operationfor specific p and fName, Just ensure avoid recursive call of
applyExtension(p, fName, ...)Let's say there is an enabled extension which name is
extensionAand has functionloadSomeAsyc.For a specific property
p,First,
applyExtension(p, fName, ...)should not be called recursively.In other words, the following trace should not occur:
Second,
extensionA.loadSomeAsyc(p, ...)should not be called recursively.In other words, the following trace should not occur:
so, just ensue no recursive call of
applyExtension(p, 'loadSomeAsyc'), two goals can be achieved at the same time.